Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content-Length header not always returned when enumerating HttpContentHeaders #102416

Merged
merged 6 commits into from
May 30, 2024

Conversation

pedrobsaila
Copy link
Contributor

Fixes #16162

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 18, 2024
@MihaZupan
Copy link
Member

Forcing the computation in the constructor is too early, derived types may not even get a chance to initialize.

Consider this example. With this change, TryComputeLength would be called before _path is set, causing an argument exception to be thrown that would then be ignored. This could mean TryComputeLength may be called more than once, which wouldn't happen before.

public sealed class FileHttpContent : HttpContent
{
    private readonly string _path;
    
    public FileHttpContent(string path)
    {
        Headers.Add("X-File-Name", Uri.EscapeDataString(Path.GetFileName(path)));
        _path = path;
    }
    
    protected override Task SerializeToStreamAsync(Stream stream, TransportContext context) =>
        SerializeToStreamAsync(stream, context, CancellationToken.None);

    protected override async Task SerializeToStreamAsync(Stream stream, TransportContext context, CancellationToken cancellationToken)
    {
        await using FileStream fs = File.OpenRead(_path);
        await fs.CopyToAsync(stream, cancellationToken);
    }

    protected override bool TryComputeLength(out long length)
    {
        length = new FileInfo(_path).Length;
        return true;
    }
}

If we want to force the value to be available during enumeration, we should force its creation when trying to enumerate and not before.

@MihaZupan MihaZupan added this to the 9.0.0 milestone May 20, 2024
@pedrobsaila
Copy link
Contributor Author

That means I need to overload GetEnumerator in HttpContentHeaders. Will this need an api review ?

@MihaZupan
Copy link
Member

These are all internal implementation details, we shouldn't need to make API changes.
For example, the GetEnumerator calls in HttpHeaders and HttpHeadersNonValidated could do the necessary type checks.

@pedrobsaila
Copy link
Contributor Author

Sorry if I'm asking too much questions, but HttpHeaders & HttpHeadersNonValidated do not know about HttpContent nor its length. I don't see how updating GetEnumerator in these two classes would help.

@MihaZupan
Copy link
Member

What I mean is that where the HttpHeaders.GetEnumerator is currently

public IEnumerator<KeyValuePair<string, IEnumerable<string>>> GetEnumerator() =>
    GetEnumeratorCore();

it could instead be

public IEnumerator<KeyValuePair<string, IEnumerable<string>>> GetEnumerator()
{
    if (this is HttpContentHeaders contentHeaders)
    {
        _ = contentHeaders.ContentLength;
    }

    return GetEnumeratorCore();
}

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking into these issues.

Thinking about it a bit more, this would still run into the issue mentioned in #102416 (comment) if the implementation tried enumerating the headers from its constructor (e.g. replace Headers.Add with Headers.ToString()).
While I doubt that's all that common, it feels too risky to just break.

We could go with just A) mentioned here: #16162 (comment)
That is, only fix the issue for built-in types.

@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented May 24, 2024

Thinking about it a bit more, this would still run into the issue mentioned in #102416 (comment) if the implementation tried enumerating the headers from its constructor (e.g. replace Headers.Add with Headers.ToString()).

The test System.Net.Http.Tests.HttpContentHeadersTest.Should_LazyLoad_ContentLengthWhenEnumeratingAndToString covers this case and passes without doing Headers.Add

@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented May 24, 2024

We could go with just A) mentioned here: #16162 (comment)

Do I make public HttpContent.Headers property virtual and override it in ByteArrayContent, StringContent, and FormUrlEncodedContent by calling TryLoadContentLength ?

@MihaZupan
Copy link
Member

I was thinking it may be as simple as Headers.ContentLength = count; in ByteArrayContent / StreamContent's ctor

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

ReadOnlyMemoryContent looks like another place where this could be applied

@MihaZupan MihaZupan merged commit d9d0e38 into dotnet:main May 30, 2024
93 checks passed
@pedrobsaila pedrobsaila deleted the 16162 branch May 30, 2024 14:12
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…tHeaders (dotnet#102416)

* Content-Length header not always returned when enumerating HttpContentHeaders

* fix failing unit test

* make loading ContentLength part of HttpHeaders and HttpHeadersNonValidated

* rework

* initialize content length in ReadOnlyMemoryContent ctor
@BrennanConroy
Copy link
Member

BrennanConroy commented May 31, 2024

Should HttpClient be removing/throwing if TransferEncoding: chunked is also set?
https://www.rfc-editor.org/rfc/rfc9112#section-6.1-15

A server MAY reject a request that contains both Content-Length and Transfer-Encoding or process such a request in accordance with the Transfer-Encoding alone. Regardless, the server MUST close the connection after responding to such a request to avoid the potential attacks.

https://www.rfc-editor.org/rfc/rfc9112#section-6.2-2

A sender MUST NOT send a Content-Length header field in any message that contains a Transfer-Encoding header field.

Noticed this because one of our tests is failing due to this update and it is also explicitly setting chunked transfer encoding
https://github.com/dotnet/aspnetcore/blob/7e6c237285cd12e93b5e882d835eb8fcec1c8ec7/src/Hosting/TestHost/test/ClientHandlerTests.cs#L183

@MihaZupan
Copy link
Member

We do that validation at the SocketsHttpHandler layer:

// Add headers to define content transfer, if not present
if (request.HasHeaders && request.Headers.TransferEncodingChunked.GetValueOrDefault())
{
if (request.Content == null)
{
return new HttpRequestException(SR.net_http_client_execution_error,
new InvalidOperationException(SR.net_http_chunked_not_allowed_with_empty_content));
}
// Since the user explicitly set TransferEncodingChunked to true, we need to remove
// the Content-Length header if present, as sending both is invalid.
request.Content.Headers.ContentLength = null;
}
else if (request.Content != null && request.Content.Headers.ContentLength == null)
{
// We have content, but neither Transfer-Encoding nor Content-Length is set.
request.Headers.TransferEncodingChunked = true;
}

I'm guessing your test is skipping that since you have a custom handler?

@BrennanConroy
Copy link
Member

Ah ok, good to know

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jun 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Content-Length' header not always returned when enumerating HttpContentHeaders
4 participants